Only reindex documents if declarations are being modified #2942
+246
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vinistock
added
enhancement
Graphite App
vinistock
commented
Nov 29, 2024
vinistock
force-pushed
the
11-29-index_documents_on_modification
branch
from
41cc250 to
826bd1a
Compare
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
2 times, most recently
from
c103d7c to
ea92bc7
Compare
andyw8
reviewed
Dec 3, 2024
vinistock
force-pushed
the
11-29-index_documents_on_modification
branch
2 times, most recently
from
2251c9d to
e5b8c20
Compare
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
from
ea92bc7 to
f299a65
Compare
vinistock
force-pushed
the
11-29-index_documents_on_modification
branch
from
e5b8c20 to
41fc7d4
Compare
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
from
f299a65 to
6fa7091
Compare
vinistock
force-pushed
the
11-29-index_documents_on_modification
branch
from
41fc7d4 to
d03faf8
Compare
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
from
6fa7091 to
bfdeee1
Compare
vinistock
changed the base branch from
11-29-index_documents_on_modification
to
graphite-base/2942
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
from
bfdeee1 to
9814d66
Compare
vinistock
force-pushed
the
graphite-base/2942
branch
from
d03faf8 to
cf1235e
Compare
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
from
9814d66 to
5f29c15
Compare
|
Just thinking out loud, but might a possible alternate approach be:
Since it's not dealing with the AST it would be faster, but it may result in more unnecessary re-indexing. Also it wouldn't correctly handle multi-line declarations. |
The issue is that this would be quite error prone. We'd be essentially trying to parse a subset of Ruby with regexes. Imagine cases like these do_something(class: "")
my_object.end
{ def: 123 }
include = 123And most of the bugs would be silent because the LSP would simply just not reindex when it's suppose to, which would make them hard to detect. |
andyw8
approved these changes
Jan 8, 2025
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
from
5f29c15 to
34510e6
Compare
Good idea, but I did the opposite. Showing when we detect that a reindex is needed instead. |
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
from
34510e6 to
5cb18b5
Compare
Merge activity
|
vinistock
deleted the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This PR tries to limit the amount of times we pay the price of reindexing the current document being modified by analyzing the context behind the last edit made to it.
Implementation
The idea of this approach is to remember the position of the last edit made to a document and the nature of the edit (insert, replace or delete).
Then, depending on the type of edit and what nodes exist under the position where the edit was made, we decide if reindexing is relevant or not.
To determine that, we look at the node under the cursor and, if it matches a node we use for indexing in the
DeclarationListener, then we consider that we need to reindex.It's not perfect, so let me know if you have other ideas on how to determine this in a way that is:
Necessarily in this order because the whole point is to reduce the performance impact of having to reindex. If detecting whether we need to do it or not is equally as slow, then there's no point in doing it.
Automated Tests
Added some tests, but we should probably add more.